Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Windows Installer: bump wix to v5 #22408

Merged
merged 1 commit into from
Jul 5, 2024
Merged

Windows Installer: bump wix to v5 #22408

merged 1 commit into from
Jul 5, 2024

Conversation

lsm5
Copy link
Member

@lsm5 lsm5 commented Apr 17, 2024 •
edited
Loading

The chocolatey tool that was fetching us wix v3 can no longer be used to
fetch wix v4+ so we had to switch to dotnet to fetch the latest wix.

This commit builds the installer with wix v5.
wix v5 is installed via the dotnet tool in the windows image itself
at containers/automation_images#354.

Going forward, the dotnet tool will also be used to build the installer.

In the process, the wix v3 files were converted to wix v4+ using wix convert followed by manual modifications along with switch to wixproj
builds with dotnet.

The GitHub Action to upload windows installer now builds the installer
using winmake.ps1.

Contributions from Mario Loriedo:

Resolves: RUN-2055

Does this PR introduce a user-facing change?

Windows installer is now built with WiX v5.

@openshift-ci openshift-ci bot added release-note-none do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 17, 2024
@lsm5 lsm5 changed the title [DO NOT MERGE] wix update [skip-ci] [DO NOT MERGE] wix update Apr 17, 2024
@lsm5 lsm5 force-pushed the wix-bump branch 2 times, most recently from 8196428 to da13f84 Compare April 17, 2024 13:50
@packit-as-a-service Packit-as-a-Service
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@lsm5 lsm5 force-pushed the wix-bump branch 2 times, most recently from 5346ecd to f6f45e9 Compare April 17, 2024 19:42
@lsm5 lsm5 changed the title [skip-ci] [DO NOT MERGE] wix update [CI:DOCS] Installer: use latest wix in windows installer Apr 17, 2024
@ashley-cui
Copy link
Member

The wix version probably also needs to be updated here, as it looks like we're hardcoding WiX 3.14

podman/contrib/win-installer/build.ps1

Line 102 in 60e0ef3

$Env:Path="$Env:Path;C:\Program Files (x86)\WiX Toolset v3.14\bin;C:\ProgramData\chocolatey\lib\mingw\tools\install\mingw64\bin;;C:\Program Files\Go\bin"

@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit f6f45e9. @martinpitt, @jelly, @mvollmer please check.

@lsm5
Copy link
Member Author

lsm5 commented Apr 17, 2024

The wix version probably also needs to be updated here, as it looks like we're hardcoding WiX 3.14

podman/contrib/win-installer/build.ps1

Line 102 in 60e0ef3

$Env:Path="$Env:Path;C:\Program Files (x86)\WiX Toolset v3.14\bin;C:\ProgramData\chocolatey\lib\mingw\tools\install\mingw64\bin;;C:\Program Files\Go\bin"

@ashley-cui ugh, do you know if we can wildcard / skip it ? I'm hoping this way we can just use the version that dotnet installs and be done with it.

@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 79506f9. @martinpitt, @jelly, @mvollmer please check.

@ashley-cui
Copy link
Member

@lsm5 I haven't tinkered with it enough to know unfortunately :(

@lsm5 lsm5 force-pushed the wix-bump branch 2 times, most recently from 723ede2 to e7884f0 Compare April 18, 2024 14:46
@packit-as-a-service Packit-as-a-Service
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 723ede2. @martinpitt, @jelly, @mvollmer please check.

@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit e7884f0. @martinpitt, @jelly, @mvollmer please check.

@packit-as-a-service Packit-as-a-Service
Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

@lsm5 lsm5 force-pushed the wix-bump branch 5 times, most recently from 3b74b3a to 71c72cc Compare April 22, 2024 19:35
@lsm5 lsm5 force-pushed the wix-bump branch 2 times, most recently from b49aa3f to a9cc854 Compare May 2, 2024 14:10
@lsm5
Copy link
Member Author

lsm5 commented Jul 4, 2024

@ashley-cui @l0rd so are we ok to skip all GHA changes in this PR? I can remove my additional commit in that case

@lsm5
Copy link
Member Author

lsm5 commented Jul 4, 2024

@ashley-cui @l0rd so are we ok to skip all GHA changes in this PR? I can remove my additional commit in that case

Talked to @l0rd privately just now. GHA doesn't have wix v5 installed by default so we definitely to account for that in this PR or else we'll only find out at release time.

I'll try to get the windows GHA to a point where we can run it on PRs if we want to, but have that part disabled by default.

@l0rd
Copy link
Member

l0rd commented Jul 5, 2024

I was following the doc on how to build the installer, but ran into this issue. Where can I get this dependency?

.\winmake.ps1 installer                                      
 Building the windows installer                                                                                         
  .\build.ps1 5.2.0 dev "C:\Users\Ashley\go\src\github.com\containers\podman\contrib\win-installer\current"               
  Required dep "MingW CC" is not installed

I have added this commit (based on this PR) to address the MingW CC problem. I will submit a dedicated PR when this one get merged.

@lsm5 lsm5 mentioned this pull request Jul 5, 2024
Closed
@lsm5 lsm5 marked this pull request as ready for review July 5, 2024 15:16
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 5, 2024
@ashley-cui
Copy link
Member

LGTM, thanks!

l0rd
l0rd approved these changes Jul 5, 2024
View reviewed changes
Copy link
Member

@l0rd l0rd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 thank you @lsm5

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jul 5, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cevich, l0rd, lsm5

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cevich
Copy link
Member

cevich commented Jul 5, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 5, 2024
@cevich
Copy link
Member

cevich commented Jul 5, 2024

Paul's #23162 will address the flaking PM Mac test. Or we can just re-run it 5 or 10 times 😩

@lsm5
Copy link
Member Author

lsm5 commented Jul 5, 2024

Paul's #23162 will address the flaking PM Mac test. Or we can just re-run it 5 or 10 times 😩

i'll rebase on that one. Thanks!

@cevich
Copy link
Member

cevich commented Jul 5, 2024

K. It should merge soon.

@lsm5 @l0rd
Windows Installer: switch to wix5
e2ff5d4 
The chocolatey tool that was fetching us wix v3 can no longer be used to
fetch wix v4+ so we had to switch to dotnet to fetch the latest wix.

This commit builds the installer with wix v5.
wix v5 is installed via the `dotnet` tool in the windows image itself
at containers/automation_images#354.

Going forward, the `dotnet` tool will also be used to build the installer.

In the process, the wix v3 files were converted to wix v4+ using `wix
convert` followed by manual modifications along with switch to wixproj
builds with dotnet.

The GitHub Action to upload windows installer now builds the installer
using winmake.ps1.

Contributions from Mario Loriedo:
- bundle setup update to wix5
- updates to build and release process scripts
Ref: #3

- small fixes to windows installer theme
Ref: #4

- Better win-installer sidebar logo
Ref: #5

Resolves: RUN-2055

Co-authored-by: Mario Loriedo <mario.loriedo@gmail.com>
Signed-off-by: Lokesh Mandvekar <lsm5@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 5, 2024
@cevich
Copy link
Member

cevich commented Jul 5, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 5, 2024
@lsm5
Copy link
Member Author

lsm5 commented Jul 5, 2024

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 5, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit c5841b0 into containers:main Jul 5, 2024
90 of 91 checks passed
@lsm5 lsm5 deleted the wix-bump branch July 5, 2024 21:45
@cevich
Copy link
Member

cevich commented Jul 8, 2024

Thanks for all your persistence on this @lsm5 😄

@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Oct 7, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Oct 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@l0rd l0rd l0rd approved these changes

@ashley-cui ashley-cui ashley-cui left review comments

@Luap99 Luap99 Luap99 requested changes

@cevich cevich cevich approved these changes

Assignees

@cevich cevich

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. machine No New Tests Allow PR to proceed without adding regression tests release-note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@lsm5 @ashley-cui @l0rd @Luap99 @cevich @edsantiago @baude @openshift-merge-robot